bpo-37702: Fix SSL certificate-store-handles leak by neonene · Pull Request #14999 · python/cpython

neonene · 2019-07-29T09:12:26Z

In Windows, CertCloseStore(hCollectionStore,[any option]) closes only hCollectionStore.
To avoid out-of-memory, CertCloseStore() shuld be called to each hSystemStore
which was added to hCollectionStore.

I think CERT_CLOSE_STORE_FORCE_FLAG is rare option and not much needed.
MSDN says CertCloseStore(handle, 0) is ideal and works fine for me.

https://bugs.python.org/issue37702

In Windows, CertCloseStore(hCollectionStore) closes only hCollectionStore. To avoid out-of-memory, CertCloseStore() shuld be called to each hSystemStore which was added to hCollectionStore.

the-knights-who-say-ni · 2019-07-29T09:12:29Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

zooba · 2019-07-29T18:54:45Z

The theory seems fine. I haven't checked the implementation - can't say when I'll get to it, but I'll try and do it before the next release.

Co-Authored-By: Ashwin Ramaswami <aramaswamis@gmail.com>

epicfaace · 2019-08-24T16:06:38Z

-       associated with the store, in this case our collection store and the
-       associated system stores. */
-    if (!CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG)) {
+    /* When a collection store and its sibling stores are closed


Do you think it would be good to keep in the definition of CERT_CLOSE_STORE_FORCE_FLAG ("forces freeing of memory for all contexts associated with the store")?

Thanks for Bool position.
For now, both 0 and CERT_CLOSE_STORE_FORCE_FLAG work fine for my pc.
I'd like to keep changes to a minimum and leave it to other ssl experts' judgement whether the flag should revert to 0 or not.

Oh, I didn't mean reverting the flag to 0, I meant, did you want to keep the part in the comment that says "CERT_CLOSE_STORE_FORCE_FLAG forces freeing of memory for all contexts associated with the store"?

MS says,
CERT_CLOSE_STORE_FORCE_FLAG:
Forces the freeing of memory for all contexts associated with the store.
This flag can be safely used only when the store is opened in a function
and neither the store handle nor any of its contexts are passed to
any called functions. For details, see ...

I think, keeping short sentences caused and will cause a misunderstanding that
this rarely used flag is always safe and releases all certificate-store-handles.
Having said that, I'm not in favor of long description in the source file.

MS doesn't recommend using CERT_CLOSE_STORE_FORCE_FLAG. https://blogs.msdn.microsoft.com/winsdk/2010/01/29/passing-the-flag-cert_close_store_force_flag-to-certclosestore-may-cause-your-application-to-crash/ And I don't want to leave confusing comments on the flag that is no longer necessary to close all handles. So, I decided to change the flag back to 0.

neonene · 2019-08-25T10:04:49Z

MS doesn't recommend using CERT_CLOSE_STORE_FORCE_FLAG.
https://blogs.msdn.microsoft.com/winsdk/2010/01/29/passing-the-flag-cert_close_store_force_flag-to-certclosestore-may-cause-your-application-to-crash/

And I don't want to leave confusing comments on the flag that is no longer necessary to close all handles.
So, I decided to change the flag back to 0.

neonene · 2019-08-27T12:01:39Z

I'm done. Sorry for the dirty PR.

bedevere-bot · 2019-08-27T20:28:29Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

neonene · 2019-08-27T22:56:12Z

Please check.
I'm relieved my flag change is not rejected.

bedevere-bot · 2019-08-27T23:35:05Z

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

Not necessary for now.

neonene · 2019-08-29T09:37:24Z

By any chance, Does this PR for Windows need further review from zooba?
Thanks to tiran and epicfaace, I think it have become readable.

neonene · 2019-08-31T20:28:27Z

I raised an alternative PR, which fix store leaks rather than handle leaks.
#15632

zooba · 2019-09-09T13:38:20Z

I'm closing this as it's unnecessary with the other fix (I verified there is no handle leak or memory leak with urlopen anymore). Thanks for your efforts! They're appreciated.

bpo-37702: Fix SSL certificate-store-handles leak

8ba1079

In Windows, CertCloseStore(hCollectionStore) closes only hCollectionStore. To avoid out-of-memory, CertCloseStore() shuld be called to each hSystemStore which was added to hCollectionStore.

neonene requested a review from tiran as a code owner July 29, 2019 09:12

the-knights-who-say-ni added the CLA not signed label Jul 29, 2019

bedevere-bot added the awaiting review label Jul 29, 2019

tiran requested a review from zooba July 29, 2019 14:37

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jul 30, 2019

epicfaace reviewed Aug 24, 2019

View reviewed changes

Comment thread Modules/_ssl.c Outdated

Comment thread Misc/NEWS.d/next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst Outdated

neonene and others added 3 commits August 25, 2019 00:40

Update 2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst

bdfd6ba

Update Modules/_ssl.c

5bbf09a

Co-Authored-By: Ashwin Ramaswami <aramaswamis@gmail.com>

Update _ssl.c

f8b70e0

epicfaace reviewed Aug 24, 2019

View reviewed changes

neonene added 4 commits August 26, 2019 00:32

bpo-37702: comment for function call to each handle

a62f754

close handles in ssl_collect_certificates()

f12ab78

comments (correct english)

5e079bf

indent, comments (correct english)

2963e03

tiran requested changes Aug 27, 2019

View reviewed changes

Comment thread Modules/_ssl.c Outdated

Comment thread Modules/_ssl.c Outdated

Comment thread Modules/_ssl.c Outdated

bedevere-bot removed the awaiting review label Aug 27, 2019

bedevere-bot added the awaiting changes label Aug 27, 2019

make the system store list a global static

e47aa04

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 27, 2019

surround #define replacement with parenthesis

9c09ed7

Not necessary for now.

add a function to close handles

b03a553

zooba closed this Sep 9, 2019

neonene deleted the bpo-37702_fix-memleak_ssl branch September 10, 2019 01:36

Uh oh!

Uh oh!

Conversation

neonene commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Jul 29, 2019

Uh oh!

zooba commented Jul 29, 2019

Uh oh!

Uh oh!

Uh oh!

epicfaace Aug 24, 2019

Choose a reason for hiding this comment

Uh oh!

neonene Aug 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epicfaace Aug 25, 2019

Choose a reason for hiding this comment

Uh oh!

neonene Aug 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neonene commented Aug 25, 2019

Uh oh!

neonene commented Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Aug 27, 2019

Uh oh!

neonene commented Aug 27, 2019

Uh oh!

bedevere-bot commented Aug 27, 2019

Uh oh!

neonene commented Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neonene commented Aug 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zooba commented Sep 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

neonene commented Jul 29, 2019 •

edited

Loading

neonene Aug 24, 2019 •

edited

Loading

neonene Aug 25, 2019 •

edited

Loading

neonene commented Aug 27, 2019 •

edited

Loading

neonene commented Aug 29, 2019 •

edited

Loading

neonene commented Aug 31, 2019 •

edited

Loading